-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Package mode] add new flag --package-mode
#1154
Conversation
autorest/codegen/templates/templates_package/CHANGELOG.md.jinja2
Outdated
Show resolved
Hide resolved
autorest/codegen/templates/templates_package/MANIFEST.in.jinja2
Outdated
Show resolved
Hide resolved
overall looks much better, thank you @msyyc! |
autorest/codegen/templates/templates_package/CHANGELOG.md.jinja2
Outdated
Show resolved
Hide resolved
test/azure/legacy/Expected/AcceptanceTests/PackageModeDataPlane/CHANGELOG.md
Outdated
Show resolved
Hide resolved
test/azure/legacy/Expected/AcceptanceTests/PackageModeDataPlane/README.md
Outdated
Show resolved
Hide resolved
...egacy/Expected/AcceptanceTests/PackageModeCustomize/azure/package/mode/aio/_configuration.py
Outdated
Show resolved
Hide resolved
test/azure/legacy/Expected/AcceptanceTests/PackageModeCustomize/template.txt
Outdated
Show resolved
Hide resolved
test/azure/legacy/Expected/AcceptanceTests/Paging/paging/operations/_paging_operations.py
Outdated
Show resolved
Hide resolved
test/vanilla/legacy/Expected/AcceptanceTests/PackageModeDataPlane/packagemode/_vendor.py
Show resolved
Hide resolved
@iscai-msft Line 76 in c9f5f14
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def getting closer @lmazuel can you take a look, left some comments ccing you. Thanks!
autorest/codegen/__init__.py
Outdated
@@ -76,6 +77,12 @@ def _validate_code_model_options(options: Dict[str, Any]) -> None: | |||
"If you want operation files, pass in flag --show-operations" | |||
) | |||
|
|||
if options["package_mode"] not in ("mgmtplane", "dataplane", None) and not Path(options["package_mode"]).exists(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it just be not in ("mgmtplane", "dataplane")
? Not sure why None
is a valid entry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path(None).exist()
will raise exception so we have to avoid that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't really do None checks for any other flag, it's still a little weird because it looks like we're making None
a valid entry (by including it with mgmtplane and dataplane)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could check None first and it will make the logic more clear
for template_name in package_files: | ||
file = template_name.replace(".jinja2", "") | ||
output_name = out_path / file | ||
if not self._autorestapi.read_file(output_name) or file == 'setup.py': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay looks good, just want to confirm with @lmazuel : are you good with us not overriding any of the files except setup.py during regeneration? This way if, for example, users update the README, we won't override when regenerating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should override the MANIFEST as well (though it's unlikely it changes, this should not be changed manually by anyone)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it and will make a override-list. Now it contains 'setup.py' and 'MAINFEST.in'
# {{ package_pprint_name }} client library for Python | ||
<!-- write necessary description of service --> | ||
|
||
## _Disclaimer_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove the disclaimer, we've already droppd support for 2.7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it's fair to remove it we're in March already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it
### Installating the package | ||
|
||
```bash | ||
python -m pip install {{ package_name }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally just do pip install {{ package_name }}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's debatable, we saw people confused because they had several Python installed, and python
and pip
were not referencing the same python interpreter. I would suggest to talk about venv instead, since this is a more efficient to ensure consistency between python and pip
|
||
def __init__(self, *, status: Optional[Union[str, "OperationResultStatus"]] = None, **kwargs): | ||
""" | ||
:keyword status: The status of the request. Possible values include: "Succeeded", "Failed", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also run at least one acceptance test on each package mode package? This way we can know if the package can be run. So for this package, you can run a paging test, and so on and so forth for the other two package mode packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add test:
test_packagemode.py for dataplane,
test_package_mode_mgmt.py for mgmtplane,
test_package_mode_customize.py for customize
|
||
```yaml $(package-mode) | ||
package-configuration: | ||
min_python_version: 3.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it's a little weird that this test includes package_name
as an entry in the custom template, since we already have flag --package-name
, maybe we could switch the packge_name
customization to a classifier customization or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed.
test/azure/legacy/specification/packagemodecustomize/template/setup.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i think with the following comments, we should be good to merge!
autorest/codegen/__init__.py
Outdated
@@ -76,6 +77,12 @@ def _validate_code_model_options(options: Dict[str, Any]) -> None: | |||
"If you want operation files, pass in flag --show-operations" | |||
) | |||
|
|||
if options["package_mode"] not in ("mgmtplane", "dataplane", None) and not Path(options["package_mode"]).exists(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't really do None checks for any other flag, it's still a little weird because it looks like we're making None
a valid entry (by including it with mgmtplane and dataplane)
{% set author_email = "azpysdkhelp@microsoft.com" %} | ||
{% set url = "https://github.com/Azure/azure-sdk-for-python/tree/main/sdk" %} | ||
{% else %} | ||
version = "{{ code_model.options.get('package_version', '0.0.0') }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @msyyc. with that check thought, we don't have to set a default value when we get package_version
from options. I think you set default value here and at least one more place, so can you change those just to code_model.options["package_version"]
?
@@ -12,7 +12,7 @@ | |||
from setuptools import setup, find_packages | |||
|
|||
|
|||
PACKAGE_NAME = "{{ package_name }}" | |||
PACKAGE_NAME = "azure-packagemode-customize" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file should be setup.py.jinja2
because it's formatted as a jinja template. Endings without .jinja2
means "keep as is" inside the custom package-mode folder, so we should be able to handle both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file should be
setup.py.jinja2
because it's formatted as a jinja template. Endings without.jinja2
means "keep as is" inside the custom package-mode folder, so we should be able to handle both
sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments are same with up and they are resolved
@@ -93,14 +98,17 @@ def _serialize_and_write_package_files_proc(**kwargs: Any): | |||
for template_name in package_files: | |||
file = template_name.replace(".jinja2", "") | |||
output_name = out_path / file | |||
if not self._autorestapi.read_file(output_name) or file == 'setup.py': | |||
if not self._autorestapi.read_file(output_name) or file in _REGENERATE_FILES: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to confirm: if a user passes any template / file inside --package-mode=my-custom-folder/
, that will override any of the templates we have in autorest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If user passes folder, autorest will use those files instead of built-in template files. This logic is to permit to override specific files when rerun autorest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay looks great @msyyc!
Want to thank you again for dealing with all of the comments and your patience, going to go ahead and merge!
…into inherit_customization * 'autorestv3' of https://github.com/Azure/autorest.python: dpg service-driven-test (#1180) prepare for release (#1181) [Package mode] add new flag `--package-mode` (#1154) hide operation group docs (#1179)
…into mypy_code * 'autorestv3' of https://github.com/Azure/autorest.python: (26 commits) dpg service-driven-test (#1180) prepare for release (#1181) [Package mode] add new flag `--package-mode` (#1154) hide operation group docs (#1179) add coverage var (#1178) improve logging (#1177) don't reformat query params for dpg next link calls (#1168) Update package.json Update ChangeLog.md default optional const parameter to none (#1171) add storage to ci checks (#1176) fail on regeneration diff (#1173) Drop Python 2.7 (#1175) Add secrets test (#1174) bump testserver version (#1172) make content type keyword only if multiple (#1167) add default value info for params to docs (#1164) update release tests to hopefully get coverage report green (#1152) fix windows: clean the outputFolderUri (#1135) Update CODEOWNERS ...
#1053